Skip to content

feat: Add animation for position switching to dock. #1121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
BLumia merged 1 commit into from
May 13, 2025
Merged

feat: Add animation for position switching to dock. #1121

BLumia merged 1 commit into from
May 13, 2025

Conversation

@wjyrich
Copy link
Contributor

@wjyrich wjyrich commented May 9, 2025 •
edited by sourcery-ai bot
Loading

as title.

pms-bug-271145

Summary by Sourcery

Add smooth animation when switching dock position to improve user experience

New Features:

  • Implement animated transition when changing dock position

Bug Fixes:

  • Fix potential issues with dock visibility during position changes

Enhancements:

  • Add PropertyAnimations to create smooth hide and show effects for dock repositioning

@sourcery-ai
Copy link

sourcery-ai bot commented May 9, 2025

Reviewer's Guide

This pull request introduces animations for dock position switching. It achieves this by adding two PropertyAnimation instances, hideDockAnimation and showDockAnimation, in main.qml. The EnumPropertyMenuItem component is modified to trigger these animations when the 'position' property is changed: the dock first animates out, the position is updated, transforms are reset, and then it animates back in.

File-Level Changes

Change Details Files
Added PropertyAnimation elements for hiding and showing the dock.
  • Defined hideDockAnimation to animate the dock moving out of view based on its current position.
  • Defined showDockAnimation to animate the dock moving into view to its new position.
  • Configured animations to target dockTransform and animate either 'x' or 'y' property depending on dock.useColumnLayout.
  • Set animation duration to 250ms with Easing.OutCubic.
  • Ensured dock.visible is set to true at the start and end of both animations.
 panels/dock/package/main.qml
Modified EnumPropertyMenuItem to orchestrate dock position change with animations.
  • When the 'position' property is changed, hideDockAnimation is started.
  • Connected a function to hideDockAnimation.onStopped signal to handle logic after the hide animation completes.
  • Inside the onStopped handler: stopped any running animations, reset dockTransform.x and dockTransform.y to 0, updated Applet.position, and then started showDockAnimation.
 panels/dock/package/main.qml
Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

sourcery-ai[bot]
sourcery-ai bot reviewed May 9, 2025
View reviewed changes
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @wjyrich - I've reviewed your changes - here's some feedback:

  • Each call to onTriggered for a position change connects a new handler to hideDockAnimation.onStopped; this could result in multiple handlers executing. Consider disconnecting the handler after it runs once.
  • Consider relocating the animation logic for position changes from EnumPropertyMenuItem to a more specialized component or handler to keep EnumPropertyMenuItem generic.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

panels/dock/package/main.qml
checked = Qt.binding(function() {
return Applet[prop] === value
})
if (prop === "position") {
Copy link

@sourcery-ai sourcery-ai bot May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Avoid creating multiple onStopped connections on hideDockAnimation.

Each time the 'position' branch runs, you reconnect the onStopped handler, leading to duplicate calls. Use a one-shot connection or disconnect the existing handler before reconnecting.

@deepin-ci-robot
Copy link

deepin-ci-robot commented May 13, 2025

deepin pr auto review

代码审查意见：

  1. 代码重复：

    • 在PropertyAnimation的from和to属性中，有重复的条件判断逻辑。可以将这部分逻辑提取到一个单独的函数中，以减少代码重复。

  2. 变量命名：

    • dockAnimation变量名不够具体，建议使用更具描述性的名称，如dockShowAnimation或dockHideAnimation。

  3. 回调函数的连接和断开：

    • 在EnumPropertyMenuItem组件的positionChangeCallback函数中，每次触发时都会断开并重新连接回调，这可能会导致性能问题。建议只在需要时连接回调，并在组件销毁时断开。

  4. 动画逻辑的清晰性：

    • dockAnimation的动画逻辑较为复杂，建议添加注释来解释每个步骤的目的和逻辑，以提高代码的可读性。

  5. dock.visible的设置逻辑：

    • 在onStopped信号处理函数中，dock.visible的设置逻辑较为复杂，建议简化这部分逻辑，使其更易于理解和维护。

  6. Qt.binding的使用：

    • 在EnumPropertyMenuItem组件中，checked属性使用了Qt.binding，这可能会导致性能问题。如果可能，建议使用更简单的绑定方式。

  7. dockTransform的初始化：

    • 在positionChangeCallback函数中，每次触发时都会重置dockTransform的x和y属性。如果dockTransform在其他地方也有使用，这可能会导致问题。建议在适当的地方初始化dockTransform。

  8. dockAnimation.stop()的使用：

    • 在positionChangeCallback函数中，每次触发时都会调用dockAnimation.stop()。如果dockAnimation已经在停止状态，这可能会导致问题。建议在调用stop()之前检查动画的状态。

  9. dockAnimation.startAnimation()的参数：

    • dockAnimation.startAnimation()函数的参数showing在EnumPropertyMenuItem组件中被硬编码为true。如果需要根据不同的条件来决定动画的方向，建议将这部分逻辑移到调用startAnimation()的地方。

  10. 代码格式和风格：

    • 代码中存在一些格式和风格上的不一致，例如缩进和空格的使用。建议统一代码格式和风格，以提高代码的可读性。

以上是针对代码审查的一些意见，希望能够对您有所帮助。

@deepin-ci-robot
Copy link

deepin-ci-robot commented May 13, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, wjyrich

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@BLumia BLumia merged commit 5e2cb87 into linuxdeepin:master May 13, 2025
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@BLumia BLumia BLumia approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wjyrich @deepin-ci-robot @BLumia